chore: 🤖 use hds keyvalue component #3027
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
didoo
left a comment
didoo
left a comment
There was a problem hiding this comment.
Left a few comments, mostly suggestion and nitpicks, apart from the one about the Error block being yielded in the wrong container, which should be fixed to make sure the errors are correctly displayed
ui/admin/tests/integration/components/form/field/key-value-test.js
Outdated
Show resolved
Hide resolved
ui/admin/app/components/form/credential-library/vault-ssh-certificate/index.hbs
Show resolved
Hide resolved
| <:row as |R|> | ||
| <R.Field as |F|> | ||
| <F.Label>{{or @keyLabel (t 'form.key.label')}}</F.Label> | ||
| {{#if (eq @keyFieldType 'select')}} |
There was a problem hiding this comment.
I'm wondering if this component is trying too hard to simplify the HDS component without adding enough convenience for the limitations it now builds in.
It seems the hideValueField is only necessary because we need to handle the option of one field or two fields but we actually should be able to handle any number of fields as HDS also supports generic fields and not just inputs which we now limit.
Would it make more sense to cut down and remove the baked in fields (as well as the switches for keyFieldType) and just let the consumer handle adding in the fields? We could just yield back the <:row as |R|> to the consumer to handle this. I also notice all the keys/values also share the same name/errors which doesn't seem right.
There was a problem hiding this comment.
Looking over this and Zed's comment, I think I would lean towards allowing the consumer to handle this by yielding back. That should give us the flexibility without this complexity I think.
There was a problem hiding this comment.
makes sense! I will yield it to the customers to handle this
There was a problem hiding this comment.
we currently don't have separate key/value error messages from the api that's why I kept them combined here.
DhariniJeeva
force-pushed
the
key-value-migration
branch
from
157e92e to
0bbffc3
Compare
DhariniJeeva
force-pushed
the
key-value-migration
branch
from
0bbffc3 to
bd628a9
Compare
DhariniJeeva
force-pushed
the
key-value-migration
branch
from
68cc696 to
08141da
Compare
DhariniJeeva
force-pushed
the
key-value-migration
branch
from
08141da to
5089a09
Compare
chore: 🤖 key value migration test: 💍 refactor tests fix: 🐛 vars, tests fix: 🐛 test fix: 🐛 include custom label fix: 🐛 empty key/value fix: 🐛 filter logic fix: 🐛 comment fix: 🐛 comments refactor: 💡 comments and helper method chore: 🤖 undo file change
Co-authored-by: Cristiano Rastelli <[email protected]>
fix: 🐛 remove extra footer and undo file change chore: 🤖 lock file fix: 🐛 test fix: 🐛 remove comment
DhariniJeeva
force-pushed
the
key-value-migration
branch
from
5089a09 to
32cceec
Compare
4 participants
Description
https://hashicorp.atlassian.net/browse/ICU-17397
This PR migrates to use hds key value inputs component.
Note: This pr only migrates a couple of forms with
textandselectinputs. The remaining forms will be migrated once this pattern is established.Screenshots (if appropriate)
sample error :
No change in response
critical options with data
critical options without data
account_claims_map with data
account_claims_map without data
How to Test
Checklist
[ ] I have added steps to reproduce and test for bug fixes in the description[ ] I have addeda11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.